Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Bump CAPI to 1.9.0 #925

Merged
merged 1 commit into from
Dec 17, 2024
Merged

Bump CAPI to 1.9.0 #925

merged 1 commit into from
Dec 17, 2024

Conversation

Danil-Grigorev
Copy link
Contributor

@Danil-Grigorev Danil-Grigorev commented Dec 13, 2024 •
edited
Loading

What this PR does / why we need it:

Depends on kubernetes-sigs/cluster-api-operator#649 merged and released first.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #922

Special notes for your reviewer:

Checklist:

  • squashed commits into logical changes
  • includes documentation
  • adds unit tests
  • adds or updates e2e tests

@Danil-Grigorev Danil-Grigorev requested a review from a team as a code owner December 13, 2024 15:38
@Danil-Grigorev Danil-Grigorev added area/dependency Issues or PRs related to dependency changes kind/chore labels Dec 13, 2024
@furkatgofurov7 furkatgofurov7 mentioned this pull request Dec 16, 2024
Closed
@furkatgofurov7
Copy link
Contributor

@Danil-Grigorev thanks for the PR! I guess you are already looking into it or noticed but there are bunch of BootstrapClusterProxy.Apply usage in the repo which needs be replaced with CreateOrUpdate

@Danil-Grigorev
Copy link
Contributor Author

Thanks @furkatgofurov7, I haven’t looked into this yet since the PR still blocked on kubernetes-sigs/cluster-api-operator#649 to be merged and released first, can you give an approval? Current issue is in transient dependency on CR from CAPI operator test package, which can be fixed only with timely release.

go: github.com/rancher/turtles/test/e2e imports
	sigs.k8s.io/cluster-api-operator/api/v1alpha1 imports
	sigs.k8s.io/controller-runtime/pkg/config/v1alpha1: module sigs.k8s.io/controller-runtime@latest found (v0.19.3), but does not contain package sigs.k8s.io/controller-runtime/pkg/config/v1alpha1

Can you elaborate how Apply and CreateOrUpdate are different? Not clear on this one.

@furkatgofurov7
Copy link
Contributor

furkatgofurov7 commented Dec 16, 2024 •
edited
Loading

Thanks @furkatgofurov7, I haven’t looked into this yet since the PR still blocked on kubernetes-sigs/cluster-api-operator#649 to be merged and released first, can you give an approval? Current issue is in transient dependency on CR from CAPI operator test package, which can be fixed only with timely release.

go: github.com/rancher/turtles/test/e2e imports
	sigs.k8s.io/cluster-api-operator/api/v1alpha1 imports
	sigs.k8s.io/controller-runtime/pkg/config/v1alpha1: module sigs.k8s.io/controller-runtime@latest found (v0.19.3), but does not contain package sigs.k8s.io/controller-runtime/pkg/config/v1alpha1

I am waiting on some dependabot PRs to pass CI and merge on capi operator before cutting a release, CAPI PR is already merged.

Can you elaborate how Apply and CreateOrUpdate are different? Not clear on this one.

The first is deprecated and is not in the e2e package anymore so you have to just replace it with latter AFAIR

@Danil-Grigorev
Copy link
Contributor Author

kubernetes-sigs/cluster-api#10442 seems to be the cause… Since we use Apply as a CreateOrPatch, this may need to be implemented in our suite.

@Danil-Grigorev Danil-Grigorev force-pushed the bump-capi-1.9 branch 7 times, most recently from ae362f6 to fa06b76 Compare December 16, 2024 17:05
@Danil-Grigorev
Bump CAPI to 1.9.0
e97f40f 
- Re-implement Apply method in local framework
- Add controller names, where needed
- Integration test: init controllers only once per all test cases

Signed-off-by: Danil-Grigorev <danil.grigorev@suse.com>

WIP: Evaluating CreateOrUpdate
Signed-off-by: Danil-Grigorev <danil.grigorev@suse.com>
@Danil-Grigorev Danil-Grigorev changed the title [WIP] Bump CAPI to 1.9.0 Bump CAPI to 1.9.0 Dec 16, 2024
salasberryfin
salasberryfin approved these changes Dec 17, 2024
View reviewed changes
Copy link
Contributor

@salasberryfin salasberryfin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Danil-Grigorev. Just to make sure I understand this correctly, does this mean we have to update our test framework to align with the deprecated Apply?

alexander-demicev
alexander-demicev approved these changes Dec 17, 2024
View reviewed changes
Copy link
Member

@alexander-demicev alexander-demicev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks a lot for doing this! nice to see unit test flakes go away

@alexander-demicev alexander-demicev merged commit 28b6aea into rancher:main Dec 17, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexander-demicev alexander-demicev alexander-demicev approved these changes

@salasberryfin salasberryfin salasberryfin approved these changes

Assignees
No one assigned
Labels
area/dependency Issues or PRs related to dependency changes kind/chore
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Bump CAPI and controller-runtime to latest
4 participants
@Danil-Grigorev @furkatgofurov7 @salasberryfin @alexander-demicev